Support Native ETH in v1 #1354
Support Native ETH in v1 #1354
Conversation
github-actions
bot
added
Component: Ethereum
Component: Smoke Tests
Component: Web
labels
alistair-singh
force-pushed
the
alistair/support-eth
branch
from
23fee16 to
c2835dd
Compare
alistair-singh
force-pushed
the
alistair/support-eth
branch
from
c2835dd to
b75401c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1354 +/- ##
==========================================
- Coverage 77.83% 76.58% -1.25%
==========================================
Files 14 17 +3
Lines 415 739 +324
Branches 76 110 +34
==========================================
+ Hits 323 566 +243
- Misses 75 162 +87
+ Partials 17 11 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
contracts/src/Gateway.sol
Outdated
| // Reimburse excess fee payment | ||
| if (msg.value > fee) { | ||
| payable(msg.sender).safeNativeTransfer(msg.value - fee); | ||
| if (msg.value > totalEther) { | ||
| payable(msg.sender).safeNativeTransfer(msg.value - totalEther); | ||
| } |
There was a problem hiding this comment.
Is this refund process still required since Gateway now is the vault of Ether? I'd suggest to remove it.
Or for security do we need to add some reentrantlock on outbound calls?
There was a problem hiding this comment.
I just wanted to port the functionality across as is. I changed the code to consider dust like we do in fees so that we do not transfer if the amount is too low. But I think it is good to refund the customer incase of any error in the input, we will not lock and hold more funds than we mint on the other side of the bridge.
There was a problem hiding this comment.
@alistair-singh I added a PR #1356 with tests to demonstrate the reentrancy behavior through the refund process.
Adding the ReentrancyGuard did enhance the security somehow. On the other hand, it will also cost a bit more gas, and it seems our bridge can't be exploited in this way. So I'm still not sure if it's necessary.
Please let me know what you think.
There was a problem hiding this comment.
Nice catch! How is ReentracyGuard different from the nonentrancy keyword in solidity 0.28 that we use in Contracts v2?
There was a problem hiding this comment.
Yeah, the Transient storage version is better.
Btw: There is an OZ version at https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/ReentrancyGuardTransient.sol
|
@vgeddes we have a Command called |
Changes
0x0000000000000000000000000000000000000000token address as Native ETH.0x00..an amount ofmsg.valueis locked in asset hub agent.Related polkadot-sdk: paritytech/polkadot-sdk#6855
TODO